Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/search improvements #304

Closed
wants to merge 49 commits into from
Closed

Conversation

kronnox and others added 30 commits February 1, 2024 15:54
Copy link

aem-code-sync bot commented Apr 5, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link

aem-code-sync bot commented Apr 5, 2024

Page Scores Audits Google
/cigaradvisor PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/cigaradvisor/search?s=padron+cigar+review PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

const SEARCH_INDEX_PATH = '/cigaradvisor/index/search-index.json';
const articleIndexData = [];

const searchIndexData = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will ever be populated? Can you run some tests and see if this keeps data between calls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bstopp you are correct...this is not getting cached between calls as we are fetching in worker... basically negating little perf improvement we could get by window.history.pushState and not reloading the page.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the worker did the fetch and returned all the results. after that we don't have to actually run a search again per-page, we just change the page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bstopp But I feel that would reset all the performance improvements we achieved...mapping entire list of search results with article-index was taking time. So, it would slow the search on first load.

Can we use Service Workers and Caching API ? Reading about them now...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, current performance is not bad as well even without this caching...these requests are cached on CDN rit?

@kailasnadh790 kailasnadh790 requested a review from bstopp April 8, 2024 15:09
@kailasnadh790 kailasnadh790 marked this pull request as draft April 8, 2024 19:40
@kailasnadh790 kailasnadh790 deleted the feat/search-improvements branch April 10, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants